Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom tooltip: add data points infos #3201

Merged
merged 11 commits into from
Oct 19, 2016
Merged

Conversation

bydooweedoo
Copy link
Contributor

Hello everyone,

Just added more informations to the custom tooltip callback argument.

In order to know which data points are matching, I added a new property dataPoints to the tooltip object passed to the custom tooltip.

This property contains an array[CustomTooltipDataPoint] describes as following:

CustomTooltipDataPoint

Name Type Description
index Number Matching point index.
datasetIndex Number Matching dataset index.
xLabel String Matching label on X axis.
yLabel String Matching label on Y axis.
pointX Number X position of matching point.
pointY Number Y position of matching point.

I also added a test case covering this new property, and a description inside the documentation under the Advanced External Tooltips section.

Please let me know if anything,
Thanks

@simonbrunel
Copy link
Member

Looks good :) I would also have updated the custom tooltips example to show how to use this new dataPoints property.

@bydooweedoo
Copy link
Contributor Author

Hi @simonbrunel,

Instead of updating example, I added a new one under samples/dataPoints-customTooltips.html :)

Not sure about the naming though ^^'

@etimberg
Copy link
Member

@bydooweedoo name is fine by me

@simonbrunel
Copy link
Member

Good for me as well

@rjmcguire
Copy link

Err, the reason people put trailing commas into code is so to
avoid diff noise in your SCM.

:D you just added more, by removing a comma that was there to avoid diff
noise...

I'm sure there is a programmer joke hiding in this situation...

On Mon, Sep 12, 2016 at 4:01 PM, Simon Brunel notifications@github.com
wrote:

Good for me as well


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#3201 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABU8CXo5qoyhKaoFXgL4ZIo5_8v5bH2Uks5qpVtUgaJpZM4Jrny1
.

@bydooweedoo
Copy link
Contributor Author

Hello @rjmcguire ,

I use trailing comma usually, but here it makes eslint just throw error on the build that's the reason why I removed them :/

Sorry about that

@rjmcguire
Copy link

Hi @bydooweedoo, I meant no offence or correction it just reminded me of an xkcd comic so I thought to share in the hopes it would humour you as much as it did me.

@simonbrunel
Copy link
Member

@rjmcguire FYI, we just changed it :)

@@ -159,6 +159,19 @@ var myPieChart = new Chart(ctx, {
});
```

In addition to all tooltip properties, a `dataPoints` property is added containing an `array[CustomTooltipDataPoint]`.

#### CustomTooltipDataPoint
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we already have a section on TooltipItems in the docs (since they are exposed to other callbacks). Can we link to that section instead of having it duplicated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, I move this documentation from Advanced to proper Chart Configuration part.

datasetIndex | Number | Matching dataset index.
xLabel | String | Matching label on X axis.
yLabel | String | Matching label on Y axis.
pointX | Number | X position of matching point.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just thinking, while the containing object isdataPoints, wouldn't x and y be better names?

dataPoints.forEach(function(point) {
    point.x; // compared to point.pointX
    point.y; // compared to point.pointY
});

Would also be more consistent with index (and not pointIndex).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think you are right :)

It makes more sense. I will change this.

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than my 1 comment on the docs, this looks good

@@ -257,6 +257,18 @@ afterBody | `Array[tooltipItem], data` | Text to render after the body section
beforeFooter | `Array[tooltipItem], data` | Text to render before the footer section
footer | `Array[tooltipItem], data` | Text to render as the footer
afterFooter | `Array[tooltipItem], data` | Text to render after the footer section
dataPoints | `Array[CustomTooltipDataPoint], data` | List of matching point informations.

#### CustomTooltipDataPoint
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the section immediately below this describes the same thing. At a minimum we should add the new x and y properties to that code snippet as well. Maybe we should consider merging these sections as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @etimberg ,

I am not sure what you want me to do here. Could you explain more ?

Which sections do you want to merge ? If you are talking about CustomTooltipDataPoint and the one just before, dataPoints is an array so it should be defined as an external section right ?

Also for x and y, which code snippet are you talking about ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant the snippet here https://github.com/chartjs/Chart.js/blob/master/docs/01-Chart-Configuration.md#tooltip-item-interface

I was also thinking to merge with the same section to that 2nd column of the last line of the table becomes Array[TooltipItemInterface]. In other words, merging the CustomTooltipDataPoint and TooltipItemInterface sections since they describe the same thing.

Also, for the tooltip item do we need the x and y properties? the user can look them up if necessary.

Copy link
Contributor Author

@bydooweedoo bydooweedoo Oct 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, yes up to you we can merge the two interface together.

I didn't know TooltipItemInterface and the CustomTooltipDataPoint were referring to the same thing, that's why I separate the two in the first place.

In my opinion, x and y properties are usefull. I am using them in several of my projects to be able to show individual tooltips at each points position.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok. That's my bad, I should have been clearer. If you can merge the doc sections and fix the merge conflict I'll merge this

@bydooweedoo
Copy link
Contributor Author

Hi @etimberg ,

I merged with current master. No more conflict.

Let me know if anything.

etimberg pushed a commit that referenced this pull request Oct 19, 2016
Expose tooltip items from tooltip model and added `x` and `y` properties to `TooltipItemInterface`
@etimberg etimberg merged commit 3bd4d28 into chartjs:master Oct 19, 2016
@simonbrunel simonbrunel modified the milestone: Version 2.4 Nov 4, 2016
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Expose tooltip items from tooltip model and added `x` and `y` properties to `TooltipItemInterface`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants